Skip to content

Nit enhance 25 03 05#2911

Merged
wwbmmm merged 6 commits into
apache:masterfrom
SamYuan1990:nitEnhance250305
Apr 11, 2025
Merged

Nit enhance 25 03 05#2911
wwbmmm merged 6 commits into
apache:masterfrom
SamYuan1990:nitEnhance250305

Conversation

@SamYuan1990

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number:
n/A

Problem Summary:
A nit fix using const instead of static new, I am not C++ expert, but it seems we just have static std::string* with a fix value in previous implications of this PR. I suppose:

  • static string with a fix value equals with const, as 1st commit.
  • After 1st commit of this PR, I notice that if a function just returns a const string, in fact, we can use a const string replace.

as I am not C++ expert, I used another 3 commits to fix the compile issue.

What is changed and the side effects?

Changed:

Side effects:

  • Performance effects(性能影响):
    I suppose those implements either previous impl for this PR or changes in this PR moves to compiler optimization.
    I searched and learned through www, I suppose there only difference between the memory location for static(previous) and const(this PR), but compiler optimization is out of my knowledge, as previous adding additional function stack....

  • Breaking backward compatibility(向后兼容性):


Check List:

  • Please make sure your changes are compilable(请确保你的更改可以通过编译).
  • When providing us with a new feature, it is best to add related tests(如果你向我们增加一个新的功能, 请添加相关测试).
  • Please follow Contributor Covenant Code of Conduct.(请遵循贡献者准则).

Signed-off-by: SamYuan1990 <yy19902439@126.com>
Signed-off-by: SamYuan1990 <yy19902439@126.com>
Signed-off-by: SamYuan1990 <yy19902439@126.com>
Signed-off-by: SamYuan1990 <yy19902439@126.com>
Signed-off-by: SamYuan1990 <yy19902439@126.com>
Signed-off-by: SamYuan1990 <yy19902439@126.com>
@SamYuan1990

SamYuan1990 commented Mar 6, 2025

Copy link
Copy Markdown
Contributor Author

the test passed on https://github.com/SamYuan1990/brpc/actions/runs/13688832022/job/38278130143?pr=33 let me test with 22.04.

@SamYuan1990

Copy link
Copy Markdown
Contributor Author

https://github.com/SamYuan1990/brpc/actions/runs/13689813245/job/38280860800?pr=34 tested my local github action, it works.

@SamYuan1990 SamYuan1990 marked this pull request as ready for review March 6, 2025 02:47
@SamYuan1990

Copy link
Copy Markdown
Contributor Author

ok, test passed, waiting for maintainer's review and feedback.

@wwbmmm

wwbmmm commented Mar 6, 2025

Copy link
Copy Markdown
Contributor

LGTM

@zhangqiongyu

Copy link
Copy Markdown
Contributor

Perhaps the original author wanted to use the lazy initialization mechanism to reduce memory usage.

@zhangqiongyu

zhangqiongyu commented Mar 7, 2025

Copy link
Copy Markdown
Contributor
const std::string& AdaptiveMaxConcurrency::UNLIMITED() {
    static const std::string s = "unlimited";
    return s;
}

const std::string& AdaptiveMaxConcurrency::CONSTANT() {
    static const std::string s = "constant";
    return s;
}

1.This way of initialization is much more common,uncontrolled use of global variables may cause dependency order problems(UB).
2.These short strings can be optimized to be stored on the stack using the SSO(Small String Optimization) mechanism of std::string.
3.The original lazy initialization mechanism is retained to reduce unnecessary memory usage.
4.It is thread-safe.

@SamYuan1990

Copy link
Copy Markdown
Contributor Author
const std::string& AdaptiveMaxConcurrency::UNLIMITED() {
    static const std::string s = "unlimited";
    return s;
}

const std::string& AdaptiveMaxConcurrency::CONSTANT() {
    static const std::string s = "constant";
    return s;
}

1.This way of initialization is much more common,uncontrolled use of global variables may cause dependency order problems(UB). 2.These short strings can be optimized to be stored on the stack using the SSO(Small String Optimization) mechanism of std::string. 3.The original lazy initialization mechanism is retained to reduce unnecessary memory usage. 4.It is thread-safe.

Hi @zhangqiongyu ,

I hope you're doing well! I wanted to check if this is a maintainer change request, as it doesn't seem to be in a CR. I also tried searching via this link but didn't find anything relevant.

I'm relatively new to C++, and I'm here to learn from everyone. If I make any mistakes, please feel free to correct me—I really appreciate it!

I believe we both agree on using const std::string instead of static std::string* s = new to ensure thread safety. However, I'm curious about whether we should use lazy initialization or not. When would be the best time to initialize this variable?

Initially, I considered using the following approach:

const std::string& AdaptiveMaxConcurrency::UNLIMITED() {
static const std::string s = "unlimited";
return s;
}

const std::string& AdaptiveMaxConcurrency::CONSTANT() {
static const std::string s = "constant";
return s;
}

But when I realized that the function simply returns a constant string, I thought about making a broader change.

From what I understand, the differences are:

  • Defining a const in a class affects the scope of the class and its instances (and possibly all child classes and their instances). It’s unique across all instances.
    
    Regarding memory usage:
    As far as I know, and as mentioned in the PR description, compiler optimization is a bit beyond my current knowledge. Since it's a short string, it might benefit from SSO (Short String Optimization). However, I'm not entirely sure how the compiler will handle it.
    
    I’m aware of the concept of dependency order problems (UB), but I lack practical experience in C++. Could you provide an example in this context?
    
    Regarding performance, when invoking this value from other parts of the code, previously it involved a function call. After changing it to a const, it becomes a direct access to a constant. From my understanding, if the compiler optimizes a function call that returns a static value, it should be equivalent to accessing a constant. If the function isn't loaded onto the stack, it might involve a page change/reload. If SSO is in play, the string would be on the stack in either case (function call or const).
    
const std::string& AdaptiveMaxConcurrency::UNLIMITED() {
static const std::string s = "unlimited";
return s;
}


I’d love to hear your thoughts on this! Thank you for your time and guidance. 😊

@zhangqiongyu

Copy link
Copy Markdown
Contributor
const std::string& AdaptiveMaxConcurrency::UNLIMITED() {
    static const std::string s = "unlimited";
    return s;
}

const std::string& AdaptiveMaxConcurrency::CONSTANT() {
    static const std::string s = "constant";
    return s;
}

1.This way of initialization is much more common,uncontrolled use of global variables may cause dependency order problems(UB). 2.These short strings can be optimized to be stored on the stack using the SSO(Small String Optimization) mechanism of std::string. 3.The original lazy initialization mechanism is retained to reduce unnecessary memory usage. 4.It is thread-safe.

Hi @zhangqiongyu ,  I hope you're doing well! I wanted to check if this is a maintainer change request, as it doesn't seem to be in a CR. I also tried searching via this link but didn't find anything relevant.  I'm relatively new to C++, and I'm here to learn from everyone. If I make any mistakes, please feel free to correct me—I really appreciate it!  I believe we both agree on using const std::string instead of static std::string* s = new to ensure thread safety. However, I'm curious about whether we should use lazy initialization or not. When would be the best time to initialize this variable?  Initially, I considered using the following approach:

const std::string& AdaptiveMaxConcurrency::UNLIMITED() {
static const std::string s = "unlimited";
return s;
}

const std::string& AdaptiveMaxConcurrency::CONSTANT() {
static const std::string s = "constant";
return s;
}

But when I realized that the function simply returns a constant string, I thought about making a broader change.  From what I understand, the differences are:

  • Defining a const in a class affects the scope of the class and its instances (and possibly all child classes and their instances). It’s unique across all instances.
    
    Regarding memory usage:
    As far as I know, and as mentioned in the PR description, compiler optimization is a bit beyond my current knowledge. Since it's a short string, it might benefit from SSO (Short String Optimization). However, I'm not entirely sure how the compiler will handle it.
    
    I’m aware of the concept of dependency order problems (UB), but I lack practical experience in C++. Could you provide an example in this context?
    
    Regarding performance, when invoking this value from other parts of the code, previously it involved a function call. After changing it to a const, it becomes a direct access to a constant. From my understanding, if the compiler optimizes a function call that returns a static value, it should be equivalent to accessing a constant. If the function isn't loaded onto the stack, it might involve a page change/reload. If SSO is in play, the string would be on the stack in either case (function call or const).
const std::string& AdaptiveMaxConcurrency::UNLIMITED() {
static const std::string s = "unlimited";
return s;
}

 I’d love to hear your thoughts on this! Thank you for your time and guidance. 😊

Hi,It's a pleasure to have a friendly discussion with you.
1.Google's programming standards clearly state that global variables should be used with caution.
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

2.Dependency order problems (UB). The snippet below is nothing todo with brpc, but what if someone wants to use this global variable in the future?
File1.cpp

#include <iostream>

// Declaration of the global variable in another file
extern int g_var2;

int g_var1 = g_var2 + 1; // Depends on the initialization order of g_var2

void func1() {
    std::cout << "func1: g_var1 = " << g_var1 << std::endl;
}

File2.cpp

#include <iostream>

// Declaration of the global variable in another file
extern int g_var1;

int g_var2 = g_var1 + 1; // Depends on the initialization order of g_var1

void func2() {
    std::cout << "func2: g_var2 = " << g_var2 << std::endl;
}

int main() {
    func1();
    func2();
    return 0;
}

3.About memory usage, lazy initialization is achieved by using functions to return static local variables, which are initialized only when needed, saving unnecessary resource overhead.
4.About performance, I have the same view with you.

In my usual projects, I prefer to use functions. In fact, I have done a lot of performance profiling, and this method has never become a performance bottleneck. Maybe what we are discussing is just some programming habits and trade-offs, but I don't think this will hinder the inclusion of PRs.

@wwbmmm wwbmmm merged commit f8eb9ec into apache:master Apr 11, 2025
yanglimingcn pushed a commit to yanglimingcn/brpc that referenced this pull request May 19, 2025
* nit fix for using const

Signed-off-by: SamYuan1990 <yy19902439@126.com>

* as it just const string refactor

Signed-off-by: SamYuan1990 <yy19902439@126.com>

* fix up

Signed-off-by: SamYuan1990 <yy19902439@126.com>

* fix up

Signed-off-by: SamYuan1990 <yy19902439@126.com>

* fix up

Signed-off-by: SamYuan1990 <yy19902439@126.com>

* clean up

Signed-off-by: SamYuan1990 <yy19902439@126.com>

---------

Signed-off-by: SamYuan1990 <yy19902439@126.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants